Skip to content

Conversation

xdoardo
Copy link
Collaborator

@xdoardo xdoardo commented Aug 28, 2025

PR rust-lang/rust#143182 added the ability to set the default address space for targets. This is of course useful for CHERI; however, some pieces of code still use the hard-coded AddressSpace::ZERO. This PR tries to reduce the usage of that constant in relevant places and when possible (that is, there is a way to get a reference to the TargetDataLayout for the target).

Most notably, this PR moves fn type_ptr from the BaseTypeCodegenMethods trait, which has no access to the TargetDataLayout for the current target, to the LayoutTypeCodegenMethods trait.

About CI Piggy-backing on this change there's a change to CI to run `./x test ui`.

@xdoardo xdoardo force-pushed the replace-address-space-zero branch from 16a783f to acf8d4b Compare August 28, 2025 14:07
@xdoardo xdoardo requested a review from seharris August 28, 2025 17:10
Copy link
Collaborator

@seharris seharris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just make sure you're okay with my comment on the CI config. If you think messing with that later will make too much mess then we might want to reconsider that change.

pull_master_script: git fetch origin master
tidy_script: CC="clang" CXX="clang++" ./x test tidy
check_script: CC="clang" CXX="clang++" ./x check
ui_test_script: CC="clang" CXX="clang++" ./x test ui
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine at this stage, but may need fiddling with once we start adding new targets (the ui suite includes tests that run code, and I think it will default to running for all targets enabled in config.toml).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, could be especially pesky for CHERIoT. I think it's okay for now, and we can adapt it to our needs later.

@xdoardo xdoardo merged commit 7471ec3 into CHERIoT-Platform:beta Sep 2, 2025
2 checks passed
@xdoardo xdoardo deleted the replace-address-space-zero branch September 11, 2025 14:46
xdoardo pushed a commit that referenced this pull request Sep 29, 2025
match clang's `va_arg` assembly on arm targets

tracking issue: rust-lang/rust#44930

For this example

```rust
#![feature(c_variadic)]

#[unsafe(no_mangle)]
unsafe extern "C" fn variadic(a: f64, mut args: ...) -> f64 {
    let b = args.arg::<f64>();
    let c = args.arg::<f64>();

    a + b + c
}
```

We currently generate (via llvm):

```asm
variadic:
    sub     sp, sp, #12
    stmib   sp, {r2, r3}
    vmov    d0, r0, r1
    add     r0, sp, #4
    vldr    d1, [sp, #4]
    add     r0, r0, #15
    bic     r0, r0, #7
    vadd.f64        d0, d0, d1
    add     r1, r0, #8
    str     r1, [sp]
    vldr    d1, [r0]
    vadd.f64        d0, d0, d1
    vmov    r0, r1, d0
    add     sp, sp, #12
    bx      lr
```

LLVM is not doing a good job. In fact, it's well-known that LLVM's implementation of `va_arg` is kind of bad, and we implement it ourselves (based on clang) for many targets already. For arm,  our own `emit_ptr_va_arg` saves 3 instructions.

Next, it turns out it's important for LLVM to explicitly start and end the lifetime of the `va_list`. In rust-lang/rust#146059 I already end the lifetime, but when looking at this again, I noticed that it is important to also start it, see https://godbolt.org/z/EGqvKTTsK: failing to explicitly start the lifetime uses an extra register.

So, the combination of `emit_ptr_va_arg` with starting/ending the lifetime makes rustc emit exactly the instructions that clang generates::

```asm
variadic:
    sub     sp, sp, #12
    stmib   sp, {r2, r3}
    vmov    d16, r0, r1
    vldr    d17, [sp, #4]
    vadd.f64        d16, d16, d17
    vldr    d17, [sp, #12]
    vadd.f64        d16, d16, d17
    vmov    r0, r1, d16
    add     sp, sp, #12
    bx      lr
```

The arguments to `emit_ptr_va_arg` are based on [the clang implementation](https://github.com/llvm/llvm-project/blob/03dc2a41f3d9a500e47b513de5c5008c06860d65/clang/lib/CodeGen/Targets/ARM.cpp#L798-L844).

r? ``@workingjubilee`` (I can re-roll if your queue is too full, but you do seem like the right person here)

try-job: armhf-gnu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants